Skip to content

refactor: use SpliceState enum instead of bool in splice functions#12365

Open
xtqqczze wants to merge 1 commit into
uutils:mainfrom
xtqqczze:splice-state
Open

refactor: use SpliceState enum instead of bool in splice functions#12365
xtqqczze wants to merge 1 commit into
uutils:mainfrom
xtqqczze:splice-state

Conversation

@xtqqczze
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze commented May 18, 2026 •
edited
Loading

Replace the ambiguous io::Result<bool> return type in pipe::splice_unbounded and pipe::splice_unbounded_broker with io::Result<SpliceState>.

This makes splice outcomes self-describing instead of relying on inverted boolean semantics (true meaning fallback required).

@oech3 sorry for the conflicts with #12359 and #12364, but I think this will make these PRs easier to review.

@xtqqczze xtqqczze mentioned this pull request May 18, 2026
Open
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 18, 2026

I don' t think this is readable. Please don't introduce unnecessary confliction.

oech3
oech3 reviewed May 18, 2026
View reviewed changes
Comment thread src/uu/cat/src/splice.rs Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 18, 2026 •
edited
Loading

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/tail-n0f (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/expand/bounded-memory is now being skipped but was previously passing.

xtqqczze
xtqqczze commented May 18, 2026
View reviewed changes
Comment thread src/uu/cat/src/splice.rs Outdated
@xtqqczze xtqqczze marked this pull request as ready for review May 18, 2026 20:23
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 18, 2026 •
edited
Loading

cc: reviewers
Please don't merge this. This breaks many PRs actually related with production quality.

@xtqqczze xtqqczze marked this pull request as draft May 18, 2026 21:01
@xtqqczze xtqqczze force-pushed the splice-state branch 2 times, most recently from 364700e to d2bffd9 Compare May 18, 2026 21:07
@xtqqczze
refactor: use SpliceState enum instead of bool in splice functions
2ce97b5 
Improve code clarity by using explicit enum values instead of boolean semantics
@xtqqczze xtqqczze marked this pull request as ready for review May 18, 2026 21:26
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 19, 2026 •
edited
Loading

If you don't like bool and flush, I just embed the required fallback to a unified function and make others private.
No need to merge this.

cc reviewers PLEASE DON'T MERGE THIS.

@oech3

This comment was marked as off-topic.

Sign in to view
@oech3
Copy link
Copy Markdown
Contributor

oech3 commented May 19, 2026

I marked this as closed by my PR since the approach embedding the write() fallback to fn is completely not compatible with enum approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@oech3 oech3 oech3 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xtqqczze @oech3